Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement LSP didSave notification and rename request #48615

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

Razoric480
Copy link
Contributor

@Razoric480 Razoric480 commented May 10, 2021

This is a companion to #48616, but for 4.0. Fixes #47609. This implements the textDocument/rename request for the language server to find symbols such as function names, variables, properties, signals and otherwise, and rename them wherever they match the original symbol.

As a companion, this also adds the textDocument/didSave notification. This is because, by the time the text changes are received by the client, none of the variables have been renamed internally, which results in error. But saving the scripts will re-trigger a parsing of all changed scripts and thus find the new property names.

Also fixes #50510 by reloading the script resource's internal state.

@akien-mga
Copy link
Member

Is this no longer relevant?

@Razoric480 Razoric480 restored the lsp-rename branch July 8, 2021 15:36
@Razoric480 Razoric480 reopened this Jul 8, 2021
@Razoric480
Copy link
Contributor Author

It still is, sorry. Got a bit zealous with branch cleaning.

@@ -310,6 +310,7 @@ class ScriptLanguage {
Ref<Script> script;
String class_name;
String class_member;
String class_path;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended that the master branch has this addition but the matching 3.x PR didn't seem to need it? #48616

That's the only reason I hadn't merged this yet as I was waiting for @vnen to check it since changing a core struct that affects all language bindings increases the scope of this PR significantly. On the other hand I see that LookupResult only seems to be used by the GDScript editor so it's probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the fundamentally there were different values between GDScript 2.0 and GDScript 1.0 that made accessing the base class for an extending class impossible in Godot 4.0, so I needed a way to reach it at the end of the day.

I made sure to only add a value, and changed nothing else about the code, so that it won't break any bindings that don't need it.

@akien-mga akien-mga merged commit 73b1f5a into godotengine:master Aug 4, 2021
@akien-mga
Copy link
Member

Thanks!

@Razoric480 Razoric480 deleted the lsp-rename branch November 30, 2021 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants